Conversation
| validator.pushPath(member.memberName()); | ||
| validator.writeBlob(member, value); | ||
| validator.popPath(); | ||
| if (value != null) { |
There was a problem hiding this comment.
Part of the contract of our shape serializers is that shapes are never supposed to serialize a null value like this. They have to use writeNull to explicitly write a null value. It seems like a bug in whatever code is serializing nulls.
|
|
||
| public void startMap(Schema schema) { | ||
| checkType(schema, ShapeType.MAP); | ||
| if (schema.isMember()) { |
There was a problem hiding this comment.
With the old implementation, I pushed and popped members names in the list/map/struct implementaitons to avoid these conditionals. Can we no do that anymore?
|
|
||
| public void validateByte(Schema schema, byte value) { | ||
| currentPresenceTracker.setMember(schema); | ||
| pushPath(schema.memberName()); |
There was a problem hiding this comment.
With the old implementation, you could validate anything, even root level shapes. It looks like this no longer supports that use case.
| writer.putContext("collections", Collections.class); | ||
| writer.write( | ||
| "this.${memberName:L} = ${?nullable}builder.${memberName:L} == null ? null : ${/nullable}${collections:T}.${wrapper:L}(builder.${memberName:L});"); | ||
| "this.${memberName:L} = builder.${memberName:L} == null ? null : ${collections:T}.${wrapper:L}(builder.${memberName:L});"); |
There was a problem hiding this comment.
The null checks are already present in the builder and we need to be able to construct a POJO with null lists during deserialization.
| writer.putContext("memberName", symbolProvider.toMemberName(member)); | ||
| var memberName = symbolProvider.toMemberName(member); | ||
| var builderMethodName = memberName; | ||
| if (isTracked || requiresNullCheck) { |
There was a problem hiding this comment.
This all feels like it really needs some comments to explain what's going on
| @Override | ||
| public ${shape:N} build() {${?hasRequiredMembers} | ||
| tracker.validate();${/hasRequiredMembers} | ||
| if (!disableChecks) { |
There was a problem hiding this comment.
What is this and how does it work? Can you now create shapes that don't validate required members are present (so for example, an int gets initialized to 0 implicitly even when it was required but not set)?
There was a problem hiding this comment.
This is used to disable the checks in the builder if we are in deserialization mode.
| job.setFailure(t); | ||
| return job.chosenProtocol().serializeError(job, t); | ||
| var failure = unwrap(t); | ||
| LOGGER.error("Failure while orchestration", failure); |
There was a problem hiding this comment.
| LOGGER.error("Failure while orchestration", failure); | |
| LOGGER.error("Failure while orchestrating", failure); |
| @@ -22,8 +22,8 @@ | |||
| public final class RequestDeserializer { | |||
There was a problem hiding this comment.
Is the javadoc above still valid? What is the change you made here?
|
|
||
| package software.amazon.smithy.java.core.serde; | ||
|
|
||
| public interface ValidatingShapeDeserializer extends ShapeDeserializer {} |
There was a problem hiding this comment.
I don't think this is used anywhere
| } | ||
| } | ||
|
|
||
| public <T extends SerializableStruct> List<ValidationError> deserializeAndValidate( |
There was a problem hiding this comment.
Is this designed to only be used by things like protocols and codecs? Or is this now the only way to create a shape that makes sure required members don't get default values?
| } | ||
| } | ||
|
|
||
| public void startList(Schema schema) { |
There was a problem hiding this comment.
Did you implement unique items validation yet?
adwsingh
force-pushed
the
server-validation
branch
2 times, most recently
from
4277887 to
035b3f1
Compare
adwsingh
force-pushed
the
server-validation
branch
from
035b3f1 to
e74ff19
Compare
3 participants
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.